Revise ABI-specific part of -preview=in #11828

kinke · 2020-10-06T15:13:20Z

This is the promised follow-up on #11000 and makes DMD exploit the specifics of the few supported ABIs (Win64, SysV x86_64, 32-bit x86). It also almost perfectly matches the proposed LDC implementation in ldc-developers/ldc#3578 (just a minor divergence for Win64 and dynamic arrays, but in that point the LDC and DMD Win64 ABI diverges in general).

dlang-bot · 2020-10-06T15:13:23Z

Thanks for your pull request and interest in making D better, @kinke! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#11828"

kinke · 2020-10-06T15:28:10Z

Pinging @Geod24 and @thewilsonator (as requested).

kinke · 2020-10-06T15:34:37Z

test/runnable/previewin.d

 // By ref because of size
-void testin2(in ulong[4] p) {}
+void testin2(in ulong[64] p) { static assert(__traits(isRef, p)); }


Increasing the size just to be on the safe side - some ABIs might be able to pass a 32-bytes aggregate in registers.

Doesn't seem wise to add a static assert here. You're forcing a behaviour when there is no spec on one.

It's not spec'd, but if a 512-bytes arg is ever to be copied, then -preview=in isn't implemented as intended.

Edit: ~~Unless we really want to please the aliasing-paranoiacs, but even then, the compiler should be able to infer that the used lvalue cannot be aliased.~~ Scratch that, even in that case, the param should still be a ref, only the callers might make an extra copy of the lvalue arg.

Do you mean, passed in memory?

I mean passed by-value, i.e., the thing that is tested that it's not.

Well, isRef would mean that no copy is made? There would still need to be spec on that though. From my reading of it, only non-trivial types must be passed by ref. Everything else either explicitly by value, or unenforceable.

It's not spec'd, but do you see any ABI on the horizon where it would be advantageous to pass half a KB by value over passing a ref? If that's the case in a few decades, the test can be adapted, until then, it makes sure the implementation is up to the expectation of eliding expensive copies.

kinke · 2020-10-06T16:01:27Z

The Win64 failures are because of in dynamic arrays now being passed by ref (as DMD uses a hidden ref for by-value T[] on Win64, i.e., copying the fat pointer and passing it by-ref), so most of this doesn't work anymore (only line 30 works):

dmd/test/compilable/previewin.d

Lines 22 to 42 in 37556c0

    
           struct FooBar 
        
           { 
        
               string toString() const 
        
               { 
        
                   string result; 
        
                   // Type inference works 
        
                   this.toString((buf) { result ~= buf; }); 
        
                   // Specifying the STC too 
        
                   this.toString((in buf) { result ~= buf; }); 
        
                   // Some covariance 
        
                   this.toString((const scope buf) { result ~= buf; }); 
        
                   this.toString((scope const(char)[] buf) { result ~= buf; }); 
        
                   this.toString((scope const(char[]) buf) { result ~= buf; }); 
        
                   return result; 
        
               } 
        
               void toString(scope void delegate(in char[]) sink) const 
        
               { 
        
                   sink("Hello world"); 
        
               } 
        
           }

This raises the question whether in should be explicitly required in these cases (or be inferred), otherwise compile errors only show up for platforms where there's a ref/val mismatch.

Geod24 · 2020-10-07T01:43:41Z

One thing to consider: At the moment, -preview=in is usable with the shipped druntime / Phobos. If we change in to apply to dynamic arrays, then we need to remove it from a few more places that expect an in delegate to be covariant with a const one, IIRC.

This raises the question whether in should be explicitly required in these cases (or be inferred), otherwise compile errors only show up for platforms where there's a ref/val mismatch.

Inference is not done at the moment, which is a big annoyance IMO. This is essentially issue 9423. In it, Kenji provides the rationale for it.
IMO inference should happen, and I actually have plans to fix this issue. The diff is essentially:

diff --git a/src/dmd/expression.d b/src/dmd/expression.d
index 546937875..1e15b8ae3 100644
--- a/src/dmd/expression.d
+++ b/src/dmd/expression.d
@@ -3923,7 +3923,7 @@ extern (C++) final class FuncExp : Expression
             auto tiargs = new Objects();
             tiargs.reserve(td.parameters.dim);

-            foreach (tp; *td.parameters)
+            foreach (idx, tp; *td.parameters)
             {
                 size_t u = 0;
                 foreach (i, p; tf.parameterList)
@@ -3939,6 +3939,9 @@ extern (C++) final class FuncExp : Expression
                 Type t = pto.type;
                 if (t.ty == Terror)
                     return cannotInfer(this, to, flag);
+                // Set storage classes on the literal to match that of the
+                // definition.
+                tf.parameterList[idx].storageClass|= pto.storageClass;
                 tiargs.push(t);
             }

Although I need to have a look as whether or not it works with nested delegates.

But if there's a difference in the ref-ness of slices between platforms, I'd rather we disallow covariance between delegates then.

kinke · 2020-10-07T14:38:17Z

Okay, covariance rules tightened to prevent this kind of implementation-dependent compile errors.

One thing to consider: At the moment, -preview=in is usable with the shipped druntime / Phobos. If we change in to apply to dynamic arrays, then we need to remove it from a few more places that expect an in delegate to be covariant with a const one, IIRC.

I wouldn't have expected to be able to use the prebuilt libs directly, without recompiling them. I don't think we should remove all possibly incompatible in from druntime/Phobos just to guarantee that compatibility. The libs should instead strive for using it (correctly) where appropriate and useful, long-term obviously. (-preview=in is something I could imagine for D3.)

Geod24 · 2020-10-07T02:00:24Z

src/dmd/typesem.d

+                // -preview=in: add `ref` storage class to suited `in` params
+                if (global.params.previewIn && (fparam.storageClass & (STC.in_ | STC.ref_)) == STC.in_)
+                {
+                    auto ts = t.baseElemOf().isTypeStruct();
+                    const isPOD = !ts || ts.sym.isPOD();
+                    if (!isPOD || target.preferPassByRef(t))
+                        fparam.storageClass |= STC.ref_;
+                }


I wish I'd knew we'd go this way earlier :)
Personally I don't mind, but it goes against @tsbockman 's review and what was discussed in the forum thread.

FWIW, I've always mentioned that it should be based on the param type only. And the implementation proposals here and for LDC make no use the of param position, and I doubt Iain will make use of it.

The only part that might be interesting is the relationship of the in parameter with other parameters, but that's something which is diagnosed much later, and not determinable from inspecting the function signature alone.

Geod24 · 2020-10-07T02:14:04Z

src/dmd/target.d

+     * Returns true if the specified parameter type (a POD) should be passed by
+     * ref for `in` params with `-preview=in`.


Can we keep a somewhat more polished documentation ? At the very least this should be a Returns section, but the fact that the parameter is only a POD could easily go in the Params documentation, and the fact that this is only for -preview=in should be more prominent.
Maybe you find this documentation redundant, but it is especially useful for people exploring the code.

Slightly reworked.

Geod24 · 2020-10-07T14:59:37Z

I wouldn't have expected to be able to use the prebuilt libs directly, without recompiling them. I don't think we should remove all possibly incompatible in from druntime/Phobos just to guarantee that compatibility. The libs should instead strive for using it (correctly) where appropriate and useful, long-term obviously. (-preview=in is something I could imagine for D3.)

Why not ? It's very possible to do, without much trouble, so why not ?
Forcing the user to recompile druntime / Phobos means that absolutely no one will trial it. See: All other -preview.

kinke · 2020-10-07T15:01:52Z

Why not ? It's very possible to do, without much trouble, so why not ?

There are probably many places in generic Phobos code where new in would make a lot of sense. Edit: Well, could be postponed for D3 or whatever, so that people can experiment with it without recompiling the libs.

Geod24 · 2020-10-07T15:08:14Z

There's currently no plan for D3 though. Also you can't even use it in generic code because it could subtly break if the compiler knows an instance was instantiated in another root module, and said root module was not compiled with -preview=in.

kinke · 2020-10-07T15:09:42Z

Also you can't even use it in generic code because it could subtly break if the compiler knows an instance was instantiated in another root module, and said root module was not compiled with -preview=in.

Mixing code compiled with and without -preview=in is a recipe for disaster anyway - at least with the new in mangling, the previous mangling (const scope [ref]) would make that probably less problematic. Edit: Oh I see ref is part of the mangle (failing test ;)) - nice. Edit2: Erm, apparently not consistently...

Geod24 · 2020-10-07T17:13:47Z

Mixing code compiled with and without -preview=in is a recipe for disaster anyway.

Well, the problem is really that Phobos is unique in that it's shipped compiled. Other libraries on code.dlang.org don't have that issue. So if we want previews to be usable, we need to find a solution for this - be it shipping multiple libs compiled with different preview switch, or carefully avoiding incompatibilities.

Edit: Oh I see ref is part of the mangle (failing test ;))

Yes, that was very intentional, exactly so that any mismatch results in at least a linker error, and not a runtime corruption.

Regarding Win64, wouldn't it be more efficient to change the D ABI to treat a slice as two parameters? The current situation, with two indirections, seems less than ideal. I assume this also applies to delegates ?

kinke · 2020-10-07T17:18:12Z

Regarding Win64, wouldn't it be more efficient to change the D ABI to treat a slice as two parameters? The current situation, with two indirections, seems less than ideal.

There's no D ABI. ;) - LDC passes it as 2 separate args. But that's more of a kludge [because it would require more work when calling druntime hooks from the compiler]. Walter wants it to be equivalent to struct Slice { size_t length; T* ptr; }, and that's what DMD does.

I assume this also applies to delegates ?

Yes, but here LDC conforms with DMD, i.e., both pass it indirectly.

Edit: These inefficiencies are exactly why -preview=in is such a nice thing (even for slices, delegates and real). But Microsoft IMO hasn't done a great job with its Win64 ABI (e.g., they only use at most 4 registers [edit2: apparently 6 with the __vectorcall extension], whereas System V a theoretical max of 6 GP + 8 SIMD registers - on the same hardware...).

kinke · 2020-10-07T17:50:18Z

So if we want previews to be usable, we need to find a solution for this - be it shipping multiple libs compiled with different preview switch, or carefully avoiding incompatibilities.

In this particular case with its deep implications, going with the 'avoiding incompatibilities' sounds like the short-term solution, although care will need to be taken for future PRs too so as not to introduce regressions.

The full potential can only be leveraged by revising the libs and shipping+linking a separate set of prebuilt libs, but that's a big task, and only feasible once enough people have been convinced about -preview=in being awesome.

kinke · 2020-10-07T19:16:13Z

Some Win64 asm produced by DMD (this PR) for:

void extReal(in real, in real);
void extSlice(in int[], in int[]);

void testReal(in real a, in real b)
{
    extReal(a, b);
}

void testSlice(in int[] a, in int[] b)
{
    extSlice(a, b);
}

dmd -m64 -preview=in:

_D7current8testRealFIKeIKeZv:
  0000000000000000: 55                 push        rbp
  0000000000000001: 48 8B EC           mov         rbp,rsp
  0000000000000004: 48 83 EC 20        sub         rsp,20h
  0000000000000008: E8 00 00 00 00     call        _D7current7extRealFIKeIKeZv
  000000000000000D: 48 83 C4 20        add         rsp,20h
  0000000000000011: 5D                 pop         rbp
  0000000000000012: C3                 ret

_D7current9testSliceFIKAiIKQeZv:
  0000000000000000: 55                 push        rbp
  0000000000000001: 48 8B EC           mov         rbp,rsp
  0000000000000004: 48 83 EC 20        sub         rsp,20h
  0000000000000008: E8 00 00 00 00     call        _D7current8extSliceFIKAiIKQeZv
  000000000000000D: 48 83 C4 20        add         rsp,20h
  0000000000000011: 5D                 pop         rbp
  0000000000000012: C3                 ret

dmd -m64:

_D7current8testRealFIeIeZv:
  0000000000000000: 55                 push        rbp
  0000000000000001: 48 8B EC           mov         rbp,rsp
  0000000000000004: 48 83 EC 20        sub         rsp,20h
  0000000000000008: DB 2A              fld         tbyte ptr [rdx]
  000000000000000A: DB 7D E0           fstp        tbyte ptr [rbp-20h]
  000000000000000D: 48 8D 55 E0        lea         rdx,[rbp-20h]
  0000000000000011: DB 29              fld         tbyte ptr [rcx]
  0000000000000013: DB 7D F0           fstp        tbyte ptr [rbp-10h]
  0000000000000016: 48 8D 4D F0        lea         rcx,[rbp-10h]
  000000000000001A: 48 83 EC 20        sub         rsp,20h
  000000000000001E: E8 00 00 00 00     call        _D7current7extRealFIeIeZv
  0000000000000023: 48 83 C4 20        add         rsp,20h
  0000000000000027: 48 8B E5           mov         rsp,rbp
  000000000000002A: 5D                 pop         rbp
  000000000000002B: C3                 ret

_D7current9testSliceFIAiIQdZv:
  0000000000000000: 55                 push        rbp
  0000000000000001: 48 8B EC           mov         rbp,rsp
  0000000000000004: 48 83 EC 20        sub         rsp,20h
  0000000000000008: 48 8B 02           mov         rax,qword ptr [rdx]
  000000000000000B: 48 8B 52 08        mov         rdx,qword ptr [rdx+8]
  000000000000000F: 48 89 45 E0        mov         qword ptr [rbp-20h],rax
  0000000000000013: 48 89 55 E8        mov         qword ptr [rbp-18h],rdx
  0000000000000017: 48 8D 55 E0        lea         rdx,[rbp-20h]
  000000000000001B: 48 8B 01           mov         rax,qword ptr [rcx]
  000000000000001E: 48 8B 49 08        mov         rcx,qword ptr [rcx+8]
  0000000000000022: 48 89 45 F0        mov         qword ptr [rbp-10h],rax
  0000000000000026: 48 89 4D F8        mov         qword ptr [rbp-8],rcx
  000000000000002A: 48 8D 4D F0        lea         rcx,[rbp-10h]
  000000000000002E: 48 83 EC 20        sub         rsp,20h
  0000000000000032: E8 00 00 00 00     call        _D7current8extSliceFIAiIQdZv
  0000000000000037: 48 83 C4 20        add         rsp,20h
  000000000000003B: 48 8B E5           mov         rsp,rbp
  000000000000003E: 5D                 pop         rbp
  000000000000003F: C3                 ret

-O seems to produce worse code in both cases:

dmd -m64 -preview=in -O:

_D7current8testRealFIKeIKeZv:
  0000000000000000: 55                 push        rbp
  0000000000000001: 48 8B EC           mov         rbp,rsp
  0000000000000004: 48 83 EC 20        sub         rsp,20h
  0000000000000008: 48 89 4D 10        mov         qword ptr [rbp+10h],rcx
  000000000000000C: 48 89 55 18        mov         qword ptr [rbp+18h],rdx
  0000000000000010: 48 8B 55 18        mov         rdx,qword ptr [rbp+18h]
  0000000000000014: 48 8B 4D 10        mov         rcx,qword ptr [rbp+10h]
  0000000000000018: E8 00 00 00 00     call        _D7current7extRealFIKeIKeZv
  000000000000001D: 48 8B E5           mov         rsp,rbp
  0000000000000020: 5D                 pop         rbp
  0000000000000021: C3                 ret

_D7current9testSliceFIKAiIKQeZv:
  0000000000000000: 55                 push        rbp
  0000000000000001: 48 8B EC           mov         rbp,rsp
  0000000000000004: 48 83 EC 20        sub         rsp,20h
  0000000000000008: 48 89 4D 10        mov         qword ptr [rbp+10h],rcx
  000000000000000C: 48 89 55 18        mov         qword ptr [rbp+18h],rdx
  0000000000000010: 48 8B 55 18        mov         rdx,qword ptr [rbp+18h]
  0000000000000014: 48 8B 4D 10        mov         rcx,qword ptr [rbp+10h]
  0000000000000018: E8 00 00 00 00     call        _D7current8extSliceFIKAiIKQeZv
  000000000000001D: 48 8B E5           mov         rsp,rbp
  0000000000000020: 5D                 pop         rbp
  0000000000000021: C3                 ret

dmd -m64 -O:

_D7current8testRealFIeIeZv:
  0000000000000000: 55                 push        rbp
  0000000000000001: 48 8B EC           mov         rbp,rsp
  0000000000000004: 48 83 EC 50        sub         rsp,50h
  0000000000000008: 48 89 5D D8        mov         qword ptr [rbp-28h],rbx
  000000000000000C: 48 89 4D 10        mov         qword ptr [rbp+10h],rcx
  0000000000000010: 48 89 55 18        mov         qword ptr [rbp+18h],rdx
  0000000000000014: 48 8B 45 18        mov         rax,qword ptr [rbp+18h]
  0000000000000018: DB 28              fld         tbyte ptr [rax]
  000000000000001A: DB 7D F0           fstp        tbyte ptr [rbp-10h]
  000000000000001D: 48 8D 55 F0        lea         rdx,[rbp-10h]
  0000000000000021: 48 8B 5D 10        mov         rbx,qword ptr [rbp+10h]
  0000000000000025: DB 2B              fld         tbyte ptr [rbx]
  0000000000000027: DB 7D E0           fstp        tbyte ptr [rbp-20h]
  000000000000002A: 48 8D 4D E0        lea         rcx,[rbp-20h]
  000000000000002E: E8 00 00 00 00     call        _D7current7extRealFIeIeZv
  0000000000000033: 48 8B 5D D8        mov         rbx,qword ptr [rbp-28h]
  0000000000000037: 48 8B E5           mov         rsp,rbp
  000000000000003A: 5D                 pop         rbp
  000000000000003B: C3                 ret

_D7current9testSliceFIAiIQdZv:
  0000000000000000: 55                 push        rbp
  0000000000000001: 48 8B EC           mov         rbp,rsp
  0000000000000004: 48 83 EC 50        sub         rsp,50h
  0000000000000008: 48 89 5D D8        mov         qword ptr [rbp-28h],rbx
  000000000000000C: 48 89 4D 10        mov         qword ptr [rbp+10h],rcx
  0000000000000010: 48 89 55 18        mov         qword ptr [rbp+18h],rdx
  0000000000000014: 48 8B 45 18        mov         rax,qword ptr [rbp+18h]
  0000000000000018: 48 8B 50 08        mov         rdx,qword ptr [rax+8]
  000000000000001C: 48 8B 00           mov         rax,qword ptr [rax]
  000000000000001F: 48 89 45 F0        mov         qword ptr [rbp-10h],rax
  0000000000000023: 48 89 55 F8        mov         qword ptr [rbp-8],rdx
  0000000000000027: 48 8D 55 F0        lea         rdx,[rbp-10h]
  000000000000002B: 48 8B 5D 10        mov         rbx,qword ptr [rbp+10h]
  000000000000002F: 48 8B 4B 08        mov         rcx,qword ptr [rbx+8]
  0000000000000033: 48 8B 03           mov         rax,qword ptr [rbx]
  0000000000000036: 48 89 45 E0        mov         qword ptr [rbp-20h],rax
  000000000000003A: 48 89 4D E8        mov         qword ptr [rbp-18h],rcx
  000000000000003E: 48 8D 4D E0        lea         rcx,[rbp-20h]
  0000000000000042: E8 00 00 00 00     call        _D7current8extSliceFIAiIQdZv
  0000000000000047: 48 8B 5D D8        mov         rbx,qword ptr [rbp-28h]
  000000000000004B: 48 8B E5           mov         rsp,rbp
  000000000000004E: 5D                 pop         rbp
  000000000000004F: C3                 ret

kinke · 2020-10-08T21:37:54Z

At the moment, -preview=in is usable with the shipped druntime / Phobos. If we change in to apply to dynamic arrays, then we need to remove it from a few more places that expect an in delegate to be covariant with a const one, IIRC.

How did you check the libs? Hacking the compiler, using some regex to wade through the source, checking mangled names in the libs, ...?

thewilsonator · 2020-10-25T01:35:06Z

Whats the status of this?

kinke · 2020-10-25T02:07:54Z

druntime/Phobos need to be rechecked, i.e., there must be no existing in passed by ref with -preview=in on any target, so that the user still doesn't have to rebuild the libs to experiment with the feature.

Geod24 · 2020-10-25T08:36:43Z

How did you check the libs? Hacking the compiler, using some regex to wade through the source, checking mangled names in the libs, ...?

Hacking the compiler and adding a printf when the promotion happens.

This is the promised follow-up on dlang#11000 and makes DMD exploit the specifics of the few supported ABIs (Win64, SysV x86_64, 32-bit x86). It also almost perfectly matches the proposed LDC implementation in ldc-developers/ldc#3578 (just a minor divergence for Win64 and dynamic arrays, but in that point the LDC and DMD Win64 ABI diverges in general).

Require the `in` storage class on both sides, just like ref/out/lazy. Doing so makes sure the code compiles with all compilers and for all targets, independent from whether `in` means `const scope` or `const scope ref`.

kinke · 2020-10-31T16:39:29Z

There are apparently many in slice and delegate params used in druntime and Phobos, which would now be passed by ref on Win64. It might be simpler to keep enforcing by-value (independent from target ABI) for slices and delegates.

kinke · 2020-10-31T22:39:42Z

With Win64 special cases in place for slices and delegates, druntime seems fine for all targets, and Phobos should be after dlang/phobos#7687.

kinke · 2020-11-01T01:40:53Z

Confirmed, libs are fine now.

Geod24 · 2020-11-01T04:54:22Z

It might be simpler to keep enforcing by-value (independent from target ABI) for slices and delegates.

Great! My main concern was really covariance with const char[], as per the toString case.
I'll give this a review within the next 12 hours.

kinke · 2020-11-01T05:16:44Z

Thx. - The output contained almost 10k warnings related to in slices/delegates, surely countless duplicates, but still... ;)

Geod24

LGTM. Few comments though:

I suppose you want to get rid of the 3rd and 5th commits (the latter being a revert for the former).
I'm not so found of tightening the covariance rules, as I wanted to make it as easy to use as possible. If a lib doesn't use in, you can still use in in your code, and pass a delegate that accepts in. However, with the current changes, this possibility is gone. Note that this was mostly important for slices. But I don't think it's a dealbreaker, so let's give it a try.
Can you merge the 4th commit into the first, and move the special case from Win64 to typesem. IMO it's much clearer to the reader to know what happens based on types than on sizes of types, which is why the original version had those:

            else if (p.type.ty == Tarray)	
                continue;	
            // Pass delegates by value to allow covariance	
            // Function pointers are a single pointers and handled below.	
            else if (p.type.ty == Tdelegate)	
                continue;

Geod24 · 2020-11-01T14:11:37Z

Also added dlang/ci#437

kinke · 2020-11-01T14:38:39Z

No real opinion on this, but I thought it might come in handy when touching the implementation again.
I prefer to keep the tightened covariance rules (primarily for target-agnostic compile errors).
I prefer keeping it as Win64 special case, because I don't intend to keep them in place forever. If -preview=in becomes the default eventually and druntime/Phobos is revised for in, we should be able to drop the special case .

Geod24 · 2020-11-01T15:01:09Z

Regarding 3, okay that makes sense.
Regarding 1, I don't think it makes sense to clutter the history like this. However, if you wish to put it behind a version (none), then I don't see an issue with that.

kinke · 2020-11-01T15:55:07Z

if you wish to put it behind a version (none)

That would require a bit of refactoring to get rid of the ugly code duplication -> both commits removed instead. ;)

Geod24

Thanks!

I expect this to be slightly more performant than the previous behavior, where a delegate was treated like a corresponding struct, passed via hidden pointer and returned via sret. The primary motivation is a smooth preparation for PR ldc-developers#3578 - in order to allow people to experiment with `-preview=in` without recompiling druntime and Phobos, `in` slices and delegates must not be passed by-ref with `-preview=in` (see dlang/dmd#11828). This would have required a special case for delegates on Win64, which is IMO better handled this way.

…ers (#3609) I expect this to be slightly more performant than the previous behavior, where a delegate was treated like a corresponding struct, passed via hidden pointer and returned via sret. The primary motivation is a smooth preparation for PR #3578 - in order to allow people to experiment with `-preview=in` without recompiling druntime and Phobos, `in` slices and delegates must not be passed by-ref with `-preview=in` (see dlang/dmd#11828). This would have required a special case for delegates on Win64, which is IMO better handled this way.

kinke requested review from ibuclaw and MartinNowak as code owners October 6, 2020 15:13

kinke force-pushed the preview_in branch from e9dd298 to ba354c6 Compare October 6, 2020 15:18

kinke commented Oct 6, 2020

View reviewed changes

kinke force-pushed the preview_in branch 2 times, most recently from f891220 to dc09dfd Compare October 6, 2020 17:05

kinke force-pushed the preview_in branch from dc09dfd to fd7fa1b Compare October 7, 2020 14:30

Geod24 reviewed Oct 7, 2020

View reviewed changes

kinke force-pushed the preview_in branch from fd7fa1b to bc53188 Compare October 7, 2020 14:58

kinke force-pushed the preview_in branch 3 times, most recently from c65c451 to ccd0dd9 Compare October 7, 2020 16:21

kinke changed the base branch from stable to master October 31, 2020 14:50

kinke added 2 commits October 31, 2020 15:55

Tighten covariance rules wrt. in params with -preview=in

46688ab

Require the `in` storage class on both sides, just like ref/out/lazy. Doing so makes sure the code compiles with all compilers and for all targets, independent from whether `in` means `const scope` or `const scope ref`.

kinke force-pushed the preview_in branch from ccd0dd9 to 46688ab Compare October 31, 2020 14:56

kinke changed the title ~~[stable] Revise ABI-specific part of -preview=in~~ Revise ABI-specific part of -preview=in Oct 31, 2020

kinke force-pushed the preview_in branch from 090b2d1 to 3a8a669 Compare October 31, 2020 16:20

kinke force-pushed the preview_in branch from e497bb2 to 3d40332 Compare November 1, 2020 01:08

Geod24 approved these changes Nov 1, 2020

View reviewed changes

Geod24 mentioned this pull request Nov 1, 2020

Minor fixes for -preview=in and targets preferring by-ref for real dlang/phobos#7687

Merged

Add Win64 -preview=in special case: by-value slices and delegates

7ae93c9

kinke force-pushed the preview_in branch from 2bea483 to 7ae93c9 Compare November 1, 2020 15:54

Geod24 approved these changes Nov 1, 2020

View reviewed changes

Geod24 added the auto-merge label Nov 1, 2020

dlang-bot merged commit 4237d3b into dlang:master Nov 1, 2020

kinke deleted the preview_in branch November 1, 2020 17:28

kinke mentioned this pull request Nov 7, 2020

Win64 ABI: Pass/return delegates like slices - in (up to) 2 GP registers ldc-developers/ldc#3609

Merged

		* Returns true if the specified parameter type (a POD) should be passed by
		* ref for `in` params with `-preview=in`.

Revise ABI-specific part of -preview=in #11828

Revise ABI-specific part of -preview=in #11828

Conversation

kinke commented Oct 6, 2020

dlang-bot commented Oct 6, 2020 • edited Loading

Bugzilla references

Testing this PR locally

kinke commented Oct 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinke Oct 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinke commented Oct 6, 2020 • edited Loading

Geod24 commented Oct 7, 2020

kinke commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Geod24 commented Oct 7, 2020

kinke commented Oct 7, 2020 • edited Loading

Geod24 commented Oct 7, 2020

kinke commented Oct 7, 2020 • edited Loading

Geod24 commented Oct 7, 2020

kinke commented Oct 7, 2020 • edited Loading

kinke commented Oct 7, 2020 • edited Loading

kinke commented Oct 7, 2020 • edited Loading

kinke commented Oct 8, 2020

thewilsonator commented Oct 25, 2020

kinke commented Oct 25, 2020

Geod24 commented Oct 25, 2020

kinke commented Oct 31, 2020

kinke commented Oct 31, 2020

kinke commented Nov 1, 2020

Geod24 commented Nov 1, 2020

kinke commented Nov 1, 2020

Geod24 left a comment

Choose a reason for hiding this comment

Geod24 commented Nov 1, 2020

kinke commented Nov 1, 2020 • edited Loading

Geod24 commented Nov 1, 2020

kinke commented Nov 1, 2020

Geod24 left a comment

Choose a reason for hiding this comment

dlang-bot commented Oct 6, 2020 •

edited

Loading

kinke Oct 6, 2020 •

edited

Loading

kinke commented Oct 6, 2020 •

edited

Loading

kinke commented Oct 7, 2020 •

edited

Loading

kinke commented Oct 7, 2020 •

edited

Loading

kinke commented Oct 7, 2020 •

edited

Loading

kinke commented Oct 7, 2020 •

edited

Loading

kinke commented Oct 7, 2020 •

edited

Loading

kinke commented Nov 1, 2020 •

edited

Loading